Code Review

Summary: Adds a refreshAssistants method to the useAssistant hook and uses it to re-read assistant definitions from disk before opening the edit dialog. This prevents overwriting externally-edited assistant files.

Findings:

• Race condition on edit click: handleEditClick calls refreshAssistants().then(() => { setEditingKey(...); setOpen(true) }). If the user clicks multiple times rapidly or navigates away before the promise resolves, stale state updates will fire. Consider adding a guard (e.g., a loading flag or aborting on unmount).
• Loading state side-effect in refreshAssistants: The method sets loading: false but never sets loading: true first. If other code depends on the loading flag, this could leave it in an unexpected state. Consider either setting loading: true at the start or not touching loading at all in this method.
• Unnecessary useEffect for editingAssistant: The useEffect that syncs editingAssistant from editingKey + assistants could be replaced by a simple derived value (useMemo or inline computation). Using useEffect + setState for derived state causes an extra render cycle and is a common React anti-pattern.
• No error feedback to user: If refreshAssistants fails, the error is silently logged to console. The user gets no indication that they may be editing stale data. Consider showing a toast/notification on failure.
• No tests added: The PR description mentions the code was human-tested but no automated tests are included for the new refreshAssistants method or the updated edit flow.

Recommendation: improve needed -- The core idea is sound and the change is small, but the unnecessary useEffect, the incomplete loading state management, and the lack of error feedback to the user should be addressed before merging.

Follow-up Review (2026-04-28)

All five concerns from the initial review have been addressed:

Minor nits (non-blocking)

1. Falsy response leaves loading: true: In refreshAssistants, if getAssistants() resolves with a falsy value, the if (assistants) guard skips the state update, leaving loading stuck at true. Consider adding an else branch: else { set({ loading: false }) }.

2. Empty useCallback deps: handleEditClick has [] as its dependency array while closing over refreshAssistants. This works because zustand store functions are stable references, but an ESLint exhaustive-deps rule would flag it.

Both are minor and can be addressed in a follow-up.

Recommendation: can merge

Follow-up Review (2026-04-29)

Reviewing new commit 725c20d4 (2026-04-28) which addresses the two minor nits from our previous review.

Code review of the new commit

Both changes are minimal and correct:

1. In useAssistant.ts, the refreshAssistants method now has three exhaustive exit paths that all set loading: false -- success with data, success with falsy response, and error. No stuck-loading-state risk remains.

2. In assistant.tsx, calling useAssistant.getState().refreshAssistants() instead of the destructured refreshAssistants is the idiomatic Zustand pattern for accessing store methods outside of reactive rendering. The useCallback deps array [] is now genuinely correct (no reactive closures).

No new issues introduced. The PR is clean and all previously raised concerns have been fully addressed across the three follow-up commits.

Recommendation: can merge

Any other feedback for this? Can this land or is there opposition?

Review: feat: re-read agent definition from disk on editing

Summary: Adds a refreshAssistants method to the useAssistant Zustand store that fetches fresh assistant data from the backend, and wires it into the edit flow so that externally-modified assistant definitions are re-read before the edit dialog opens.

Positive observations

• Solves a real user-facing issue where stale cached data could be overwritten on save.
• The refreshRequestRef pattern correctly guards against race conditions from rapid clicks -- if a newer refresh is triggered, the stale callback is discarded.
• Good test coverage: the new refreshAssistants method has tests for success, loading state, and error (toast) paths.
• Error handling with toast.error gives the user feedback if the refresh fails.

Issues and observations

1. handleEditClick does not show a loading indicator. The user clicks Edit, then nothing visible happens until refreshAssistants() resolves. If the backend is slow, this feels like a broken button. Consider showing a spinner or disabling the button during the refresh.
2. Error path silently swallows the edit action. If refreshAssistants() rejects, the toast fires but the edit dialog never opens. The user may not connect the toast to the fact that the edit button "did nothing." Consider opening the dialog with the cached data as a fallback, or at least making the toast message more actionable.
3. Minor: vi.hoisted mock pattern. Using vi.hoisted for the mocks is correct for module-level hoisting, but the afterEach that calls mockToast.mockClear() is redundant since beforeEach already calls vi.clearAllMocks(). Not harmful, but unnecessary.
4. Whitespace-only changes in assistant.tsx. The trailing comma additions and reformatting in the destructured import are noise that inflate the diff. Not a blocker but worth noting for future PRs.

Prior review status

I see this PR has gone through two prior review rounds and all feedback was addressed. The author is asking if it can land.

Recommendation: can merge

On the feedback points:

• Re: 1. This should be an extremely minor time delta by human perception standards. I am okay with not increasing complexity for this UI sugar. If the system is slow enough that a JSON parse takes meaningful time there will be other signs to the user to explain this behavior.
• Re: 2. Consider opening the dialog with the cached data as a fallback defeats the purpose of the design. This would allow an overwrite with stale data. The toast is plenty actionable, since repeated use will succeed or keep producing the visible error.
• Re: 4. The whitespace changes are around my change (web-app/src/routes/settings/assistant.tsx:~33) - and are removing noise without confusing the diff, while keeping up to date with lint standards in a file I touch. I believe the change to be a net positive as part of this PR and negligible to noise review noise.

/cc @qnixsynapse - You seem to be the prime contact for human attention. Sorry for the direct ping - are we good here?

Follow-up Review (2026-05-05)

Reviewing @sshirokov's responses to our previous review and the latest commit (bebc098, 2026-05-04).

Author responses to non-blocking notes

Current state of the code

The final diff is clean and well-structured:

• refreshAssistants in useAssistant.ts: Three exhaustive exit paths all set loading: false. Error feedback via toast.error. Correct pattern.
• handleEditClick in assistant.tsx: Uses refreshRequestRef counter to guard against stale callbacks from rapid clicks. Calls useAssistant.getState().refreshAssistants() so the empty useCallback([]) deps are correct.
• Tests cover success, loading state, and error paths. Mock hoisting is properly done via vi.hoisted().

No new issues. All previously raised concerns have been addressed across the iterative review rounds. The author's pushback on the remaining non-blocking notes is well-reasoned.

Recommendation: can merge

cc/ @Minh141120 maybe?

🥺
👉👈

Weekly bump 🙏

@qnixsynapse, @Minh141120 @louis-jan - Can this merge? It fixes a legitimate issue I ran directly into and had been sitting with positive feedback for a few weeks.

Re-pinging: @qnixsynapse @Minh141120 @louis-jan - can this merge or should this close.

Sometimes, I feel like you're ignoring me on purpose, and it hurts my feelings.

Follow-up review (new activity detected)

Since our last detailed review (2026-05-05), new activity includes:

• Author @sshirokov has rebased the branch (all commits now show committer date 2026-05-27) and asked for merge multiple times (2026-05-06, 2026-05-13, 2026-05-15, 2026-05-22, 2026-05-29)
• The 5-commit series is now cleanly rebased onto a recent main
• No code changes since our last review -- only the rebase

Current state

All concerns from our initial and follow-up reviews have been addressed across the 5 commits:

1. Race condition on rapid edit clicks -- guarded with refreshRequestRef counter
2. loading state properly set to true at start of refreshAssistants
3. Removed unnecessary useEffect for editingAssistant -- uses inline assistants.find()
4. Error feedback via toast.error instead of console.error
5. Stuck loading state guarded with else { set({ loading: false }) } branch
6. Clean useCallback deps by calling useAssistant.getState().refreshAssistants() directly
7. Test mock hoisting fixed with vi.hoisted()
8. Redundant afterEach mockClear removed

The code is clean, well-tested (3 new test cases), and solves a real user-facing issue. The PR has been waiting for maintainer merge approval for over a month.

Recommendation: can merge

Status check (2026-06-06)

No new commits or code changes since the last review on 2026-06-03. The branch is rebased on main and all previously raised concerns have been resolved.

Quick recap of the change

• Adds refreshAssistants() to the useAssistant Zustand store, which re-reads assistant definitions from disk via getAssistants().
• Wires it into the edit-click flow in assistant.tsx so the edit dialog always opens with the latest on-disk data, preventing overwrites of externally-modified files.
• Guards against race conditions with a refreshRequestRef counter pattern.
• Includes 3 new test cases covering success, loading state, and error paths.

Files changed (3)

No outstanding issues. This is a well-scoped, well-tested fix for a real user-facing bug. The author has been patiently waiting for maintainer approval since late April.

Recommendation: can merge